Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support wchar_t in argv in windows #3547

Closed
wants to merge 1 commit into from

Conversation

npes87184
Copy link
Contributor

This MR will fix #2932.

There are various approaches that can be used to address this issue, such as the usage of wmain with link flag -municode. Only the applied method, however, was successful.

Signed-off-by: Yu-Chen Lin [email protected]

@rom1v
Copy link
Collaborator

rom1v commented Oct 23, 2022

Awesome, thank you 👍

I will test soon.

app/src/main.c Outdated
int wargc;
wchar_t **wargv = CommandLineToArgvW(GetCommandLineW(), &wargc);

if (argc != wargc) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible? Shouldn't it be an assertion?

Copy link
Contributor Author

@npes87184 npes87184 Oct 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my point of view, it is not possible. However, it doesn't indicate it in the official document.

rom1v pushed a commit that referenced this pull request Oct 23, 2022
PR #3547 <#3547>
Fixes #2932 <#2932>

Signed-off-by: Yu-Chen Lin <[email protected]>
Signed-off-by: Romain Vimont <[email protected]>
@rom1v
Copy link
Collaborator

rom1v commented Oct 23, 2022

I think this is not correct to modify argv passed to main (it might be non-writable memory IIRC).

I added error handling, and to keep things "separate", I moved the code to a main "wrapper". That way, the "main" main (main_scrcpy) always receives argc/argv in UTF-8 as expected. This implementation does not even require argc == wargc, so I removed the check.

Here is a branch: pr3547.

Please review and test if that still works (currently, I could not test on Windows).

@npes87184 npes87184 force-pushed the support_wchar_t_in_argv branch from a18318e to b1dddc8 Compare October 23, 2022 11:44
@npes87184
Copy link
Contributor Author

I tested and it worked well.

However, I found that the elements in array argv_utf8 forgotten to free.
I slightly added related logic, please check on it.

  for (int i = 0; i < wargc; ++i) {
      free(argv_utf8[i]);
  }

Also in the windows main, the argc does not use and it will cause compiler complains.

../app/src/main.c: In function ‘main’:
../app/src/main.c:89:10: warning: unused parameter ‘argc’ [-Wunused-parameter]
   89 | main(int argc, char *argv[]) {
      |      ~~~~^~~~

Additionally, I saw that you changed __WINDOWS__ to _WIN32 . I discovered that the code base still contains some of __WINDOWS__. Do you want them all taken out? I can change all of them if it is intentional.

rom1v pushed a commit that referenced this pull request Oct 23, 2022
PR #3547 <#3547>
Fixes #2932 <#2932>

Signed-off-by: Yu-Chen Lin <[email protected]>
Signed-off-by: Romain Vimont <[email protected]>
rom1v pushed a commit that referenced this pull request Oct 23, 2022
PR #3547 <#3547>
Fixes #2932 <#2932>

Signed-off-by: Yu-Chen Lin <[email protected]>
Signed-off-by: Romain Vimont <[email protected]>
@rom1v
Copy link
Collaborator

rom1v commented Oct 23, 2022

However, I found that the elements in array argv_utf8 forgotten to free.

Good catch 👍

Also in the windows main, the argc does not use and it will cause compiler complains.

OK, I added:

    (void) argc;
    (void) argv;

(yes, also argv, which was used mistakenly instead of argv_utf8 in the first version)

Here is a branch: pr3547.2.

Additionally, I saw that you changed __WINDOWS__ to _WIN32

Yes, __WINDOWS__ is a SDL define. Often, we can just use the "native" one _WIN32. Except maybe in icon.c which also uses __APPLE__ (but the windows ifdef can probably be completely removed here, since the workaround seems not to work on Windows anyway #3458).

@npes87184
Copy link
Contributor Author

LGTM in pr3547.2 branch.

@rom1v
Copy link
Collaborator

rom1v commented Oct 25, 2022

👍 merged into dev: 48bb6f2

@rom1v rom1v closed this Oct 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants